fix(tui): resolve terminal theme mode from every signal instead of guessing dark - #1152
fix(tui): resolve terminal theme mode from every signal instead of guessing dark#1152sahrizvi wants to merge 5 commits into
Conversation
…ssing dark Third attempt at the same defect (#617 → #704 → #736): code rendered in near-white on a light terminal, readable in VS Code but not in the terminal itself. The first two fixes adjusted colour values, which is why neither held. The actual defect is in `app.tsx`, where the mode-resolution chain ended in a hardcoded fallback: const envMode = detectModeFromCOLORFGBG(process.env.COLORFGBG) const mode = envMode === "light" ? "light" : ((await renderer.waitForThemeMode(1000)) ?? "dark") Apple Terminal — the client named in #736's metadata, alongside "macOS Appearance: Light" — sets no `COLORFGBG` and does not reliably answer the OSC 11 background query. Both signals are therefore absent, the chain returns "dark", and a light-background user gets the dark palette no matter how its colours are tuned. That is not a palette bug, so palette fixes could not close it. Changes: - `resolveInitialMode()` encodes the whole chain as one pure function, ordered by how well each signal describes *this terminal window*: COLORFGBG, then the OSC 11 reply, then OS appearance, then dark as a genuine last resort. A dark-profile terminal under a light system theme stays dark. - `detectSystemAppearance()` adds the signal that was missing. macOS sets `AppleInterfaceStyle` to "Dark" in dark mode and leaves it *unset* in light mode, so `defaults` exiting non-zero is the light answer rather than a failure; only ENOENT or a timeout is treated as "unknown". Every report of this bug came from darwin. - The call site now honours a dark `COLORFGBG` too. It previously kept only "light", so a terminal that had already reported a dark background still paid the full one-second OSC timeout before agreeing with it. `detectModeFromCOLORFGBG` carried a comment saying it was "extracted from app.tsx for direct test coverage (#704)" but had no tests at all. It does now. Verified by mutation rather than by the suite going green: restoring the old hardcoded fallback fails the test named for #736, discarding a dark COLORFGBG fails the precedence test, and letting OS appearance outrank the terminal's own background fails two more. Scope note: this closes the colour-mode family. #404 (garbled ASCII logo), #609 (malformed layout) and #737 (unexpected CJK glyphs) were grouped with it during triage, but they are glyph-width and encoding problems rather than colour, and need separate work. #809 (dark text on a dark box) is plausibly the same misdetection, but the report carries no terminal details, so it is referenced rather than closed. Tests: 13 new, 280 tui pass (1 pre-existing failure unrelated, identical on main). Closes #736 Refs #809 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… fallback assuming dark Reworked after a codex audit of the whole colour-legibility class, which found the first attempt was aimed at the less important of two layers and that several of its tests proved less than their names claimed. ## Direct-run renderer (the layer the audit ranked higher) `resolveRunTheme` returned a hardcoded dark `RUN_THEME_FALLBACK` on both failure exits, so a light terminal whose palette query failed got dark panels. Worse, that fallback's `text` prefers the terminal's *own* default foreground, which on a light terminal resolves to black — a black foreground over a hardcoded `#0f172a` panel is literally dark text in a dark box, the symptom reported in #809. The fallback is now built per mode and memoized, and both exits resolve a mode first. The dark instance is still the same object, so callers comparing it by identity are unaffected. ## Startup detection - Precedence corrected. OSC 11 describes *this* window right now; `COLORFGBG` is inherited and survives ssh, tmux, sudo and profile changes. The previous ordering let a stale env var override a live answer. COLORFGBG now only shortens the OSC wait (250ms instead of 1s), which keeps #704's startup win without trading away correctness. - The appearance probe no longer reports "light" for every failure. macOS leaves `AppleInterfaceStyle` unset in light mode and `defaults` says so explicitly; that diagnostic is the light answer, while EACCES, EMFILE, ENOENT, a signal or a timeout mean unknown. Guessing light on those produces the inverse of the bug being fixed. - It invokes `/usr/bin/defaults`, so a different `defaults` earlier on PATH cannot answer a question about macOS appearance. - It does not run over ssh, where the appearance belongs to the remote host rather than the terminal the user is looking at, nor in CI. ## Tests The audit named six tests that overclaimed. `execFile` is now injectable and the probe tests use a spy, so "does not spawn" is asserted rather than assumed, and every failure branch is driven directly. The regression test is renamed for the shape it actually covers instead of implying end-to-end coverage it does not have. Seven mutants were confirmed to fail: old precedence, missing ssh guard, missing CI guard, relative `defaults`, any-failure-means-light, always-dark direct-run fallback, and a light fallback whose panel is still dark. ## Scope Claims only what the code supports. The audit found #617 was missing Markdown `fg` and code-block background (fixed separately in 5ae5b79), #704 bundled three changes with no way to attribute the fix, and #404/#609/#737/#116 are glyph-width, encoding or layout problems rather than colour. #736 is the best-supported colour-mode case but remains conditional, so it is referenced, not closed. Tests: 17 detection, 9 direct-run theme, 284 tui, 187 cli/run. Typecheck clean. Refs #736 Refs #809 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe TUI now detects terminal mode from OSC responses, ChangesTerminal theme detection
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to This change improves terminal theme selection and light-mode fallback behavior without introducing new interfaces, permissions, dependencies, or deployment behavior; no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant TUI
participant Terminal
participant SystemAppearance
participant ThemeResolver
participant RunFooter
TUI->>Terminal: Query OSC 11 and read COLORFGBG
TUI->>SystemAppearance: Query appearance if terminal signals are absent
SystemAppearance-->>TUI: Return light, dark, or unknown
TUI->>ThemeResolver: Resolve initial mode
ThemeResolver-->>TUI: Return mode-specific fallback theme
ThemeResolver->>RunFooter: Provide resolved theme
RunFooter->>RunFooter: Skip recognized fallback themes during palette refresh
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description follows the required template. It identifies issue Full details: Out of Scope Changes checkExplanation The theme detection and fallback changes are in scope for [
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| * This is the signal that was missing: every report of this bug came from | ||
| * darwin, on a terminal that answers neither of the cheaper probes. | ||
| */ | ||
| /** Minimal shape of `child_process.execFile`, injectable so tests can drive every branch. */ |
There was a problem hiding this comment.
SUGGESTION: The detectSystemAppearance doc comment (lines 61-68) is now orphaned — inserting ExecFileLike here detaches it from the function it documents.
The macOS-specific explanation (AppleInterfaceStyle set to "Dark" in dark mode and absent in light mode, non-zero exit = light) is meant to describe detectSystemAppearance, but it now sits directly above the ExecFileLike type. Move that doc comment down to immediately precede detectSystemAppearance (or fold it into that function's JSDoc), so the two descriptions stop pointing at the wrong declarations.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| // silently reported as light. | ||
| const err = error as NodeJS.ErrnoException & { killed?: boolean; status?: number | null; stderr?: string } | ||
| const notFound = /does not exist/i.test(String(err.stderr ?? err.message ?? "")) | ||
| const clean = err.code === undefined && err.killed !== true |
There was a problem hiding this comment.
SUGGESTION: The clean clause is dead code that contradicts the contract stated in the comment above it.
For a real execFile error, code is always populated: the errno string on spawn failure (ENOENT/EACCES/EMFILE/...), the numeric exit code on a non-zero exit, or null when killed is true. So err.code === undefined never holds, and this clause never fires. If it ever did fire (e.g. a future error shape without code), it would silently report "light" for an unknown failure — exactly what the comment says must not happen. If the intent is to treat a clean non-zero exit (missing key) as light, match the exit status (typeof err.code === "number" / err.status === 1) instead; otherwise remove the clause.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| * the direct-run and scrollback renderer. | ||
| */ | ||
| function fallbackMode(renderer: CliRenderer): "dark" | "light" { | ||
| return resolveInitialMode({ |
There was a problem hiding this comment.
SUGGESTION: fallbackMode omits the OS-appearance signal, so the direct-run renderer does not actually "agree" with the TUI startup path it shares resolveInitialMode with.
The startup path feeds appearance from detectSystemAppearance() into resolveInitialMode, but this fallback only passes COLORFGBG and themeMode. On a light Apple Terminal (no COLORFGBG, no OSC 11 reply) a failed palette query still resolves to "dark" and reproduces the dark-on-dark symptom (#809) this PR targets. Consider threading appearance through here too (which would require making this path async), or note the limitation explicitly.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (4 files)
Fix these issues in Kilo Cloud Previous Review Summaries (2 snapshots, latest commit e452112)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit e452112)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (6 files)
Fix these issues in Kilo Cloud Previous review (commit 0306bf0)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (6 files)
Reviewed by deepseek-v4-pro · Input: 55.9K · Output: 36.6K · Cached: 828.2K Review guidance: REVIEW.md from base branch |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/tui/src/app.tsx">
<violation number="1" location="packages/tui/src/app.tsx:278">
P2: When a valid but stale `COLORFGBG` is present and OSC 11 takes longer than 250 ms, the live terminal answer is discarded and the stale value determines the theme. Keep the OSC probe alive through the full response deadline before falling back to `COLORFGBG`.</violation>
</file>
<file name="packages/opencode/src/cli/cmd/run/theme.ts">
<violation number="1" location="packages/opencode/src/cli/cmd/run/theme.ts:709">
P2: When a runtime palette refresh fails in light mode, this returns a distinct light fallback that `footer.ts` does not recognize as a fallback. The footer then replaces the last known-good theme; preserve the existing theme for either per-mode fallback.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // now. COLORFGBG only buys a shorter wait: with a usable hint in hand we | ||
| // can stop waiting sooner, which keeps #704's startup win without | ||
| // letting a stale env var override a live answer. | ||
| const oscMode = (await renderer.waitForThemeMode(envMode ? 250 : 1000)) ?? null |
There was a problem hiding this comment.
P2: When a valid but stale COLORFGBG is present and OSC 11 takes longer than 250 ms, the live terminal answer is discarded and the stale value determines the theme. Keep the OSC probe alive through the full response deadline before falling back to COLORFGBG.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/tui/src/app.tsx, line 278:
<comment>When a valid but stale `COLORFGBG` is present and OSC 11 takes longer than 250 ms, the live terminal answer is discarded and the stale value determines the theme. Keep the OSC probe alive through the full response deadline before falling back to `COLORFGBG`.</comment>
<file context>
@@ -265,9 +265,19 @@ export const run = Effect.fn("Tui.run")(function* (input: TuiInput) {
+ // now. COLORFGBG only buys a shorter wait: with a usable hint in hand we
+ // can stop waiting sooner, which keeps #704's startup win without
+ // letting a stale env var override a live answer.
+ const oscMode = (await renderer.waitForThemeMode(envMode ? 250 : 1000)) ?? null
+ const appearance = oscMode || envMode ? null : await detectSystemAppearance()
+ const mode = resolveInitialMode({ colorfgbg: process.env.COLORFGBG, osc: oscMode, appearance })
</file context>
| const oscMode = (await renderer.waitForThemeMode(envMode ? 250 : 1000)) ?? null | |
| const oscMode = (await renderer.waitForThemeMode(1000)) ?? null |
| const bg = colors.defaultBackground ?? colors.palette[0] | ||
| if (!bg) { | ||
| return RUN_THEME_FALLBACK | ||
| return runThemeFallback(fallbackMode(renderer)) |
There was a problem hiding this comment.
P2: When a runtime palette refresh fails in light mode, this returns a distinct light fallback that footer.ts does not recognize as a fallback. The footer then replaces the last known-good theme; preserve the existing theme for either per-mode fallback.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/cli/cmd/run/theme.ts, line 709:
<comment>When a runtime palette refresh fails in light mode, this returns a distinct light fallback that `footer.ts` does not recognize as a fallback. The footer then replaces the last known-good theme; preserve the existing theme for either per-mode fallback.</comment>
<file context>
@@ -660,7 +706,7 @@ export async function resolveRunTheme(renderer: CliRenderer): Promise<RunTheme>
const bg = colors.defaultBackground ?? colors.palette[0]
if (!bg) {
- return RUN_THEME_FALLBACK
+ return runThemeFallback(fallbackMode(renderer))
}
</file context>
Marker Guard failed on #1152: theme.ts is an upstream-shared file, so custom code there must be fenced to survive an upstream merge overwriting it. The mode-aware fallback added in this branch was unmarked. Six regions are now fenced: the shared-resolver import, the per-mode seed, the memoized per-mode fallback theme, the mode probe, and both failure exits in resolveRunTheme. Verified with the same command CI runs: bun run script/upstream/analyze.ts --markers --base origin/main --strict Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
2 similar comments
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
Addresses the open review findings on this PR. `fallbackMode` fed `resolveInitialMode` only COLORFGBG and the OSC reply, so the direct-run renderer did not actually agree with the startup path it shares that function with. On a light Apple Terminal — no COLORFGBG, no OSC 11 answer — a failed palette query still resolved to "dark" and repainted a light terminal dark, which is the #809 symptom this branch exists to remove. It now consults `detectSystemAppearance()`, but only when both cheap signals came back empty, so the `defaults` spawn stays off the common path. `footer.ts` keeps the last known-good theme when a runtime palette refresh fails, and detected that by comparing against `RUN_THEME_FALLBACK`. Once the fallback became per-mode, a light terminal produced a different instance, the identity check missed, and the footer replaced a good theme with the fallback. `isRunThemeFallback` tests membership in the memo map instead. `detectSystemAppearance` had a `clean` clause resolving "light" for an error with no `code` that was not killed. execFile never produces that shape — it sets `code` to the errno string on spawn failure and to the exit status otherwise — and had it fired it would have reported light for an unknown failure, which the comment directly above it forbids. Unknown now stays null so the caller can keep looking. The palette-failure test pinned the dark instance by identity, which re-encoded the behaviour being fixed and fails on a light-mode runner. It now asserts that a failed lookup yields *a* fallback, with a separate case keeping the dark instance pinned for an explicit dark signal. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018fJ3X7pcGT4R9yzjsJnqsV
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/test/cli/run/theme.test.ts">
<violation number="1" location="packages/opencode/test/cli/run/theme.test.ts:79">
P3: This new test duplicates an existing test already in this file: both are titled "a dark terminal still gets the dark fallback" and both assert that `resolveRunTheme(renderer({ fail: true, themeMode: "dark" }))` returns the dark fallback instance. Since `RUN_THEME_FALLBACK === runThemeFallback("dark")` (memoized identity), they are identical checks, and the same-named test at line 101 runs the same assertion. Duplicate registered test names are confusing and add no coverage. Drop one of the two (or merge them into a single dark-signal assertion).</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| expect(isRunThemeFallback(theme)).toBe(true) | ||
| }) | ||
|
|
||
| test("a dark terminal still gets the dark fallback", async () => { |
There was a problem hiding this comment.
P3: This new test duplicates an existing test already in this file: both are titled "a dark terminal still gets the dark fallback" and both assert that resolveRunTheme(renderer({ fail: true, themeMode: "dark" })) returns the dark fallback instance. Since RUN_THEME_FALLBACK === runThemeFallback("dark") (memoized identity), they are identical checks, and the same-named test at line 101 runs the same assertion. Duplicate registered test names are confusing and add no coverage. Drop one of the two (or merge them into a single dark-signal assertion).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/test/cli/run/theme.test.ts, line 79:
<comment>This new test duplicates an existing test already in this file: both are titled "a dark terminal still gets the dark fallback" and both assert that `resolveRunTheme(renderer({ fail: true, themeMode: "dark" }))` returns the dark fallback instance. Since `RUN_THEME_FALLBACK === runThemeFallback("dark")` (memoized identity), they are identical checks, and the same-named test at line 101 runs the same assertion. Duplicate registered test names are confusing and add no coverage. Drop one of the two (or merge them into a single dark-signal assertion).</comment>
<file context>
@@ -60,7 +66,21 @@ function spread(color: RGBA) {
+ expect(isRunThemeFallback(theme)).toBe(true)
+})
+
+test("a dark terminal still gets the dark fallback", async () => {
+ // The mode-aware path must not have inverted anything: given an explicit
+ // dark signal the fallback is still the dark instance callers compare by
</file context>
| // OSC 11 reply — it still resolved "dark" and repainted a light terminal | ||
| // dark, which is the #809 symptom this change exists to remove. The probe | ||
| // spawns `defaults`, so it stays behind the two free signals. | ||
| const appearance = osc || detectModeFromCOLORFGBG(colorfgbg) ? null : await detectSystemAppearance() |
There was a problem hiding this comment.
SUGGESTION: detectSystemAppearance() spawns defaults on the footer's palette-refresh failure path, where the answer is discarded.
resolveRunTheme is shared by the direct-run startup (runtime.lifecycle.ts:198, which consumes the fallback) and the TUI footer's handlePalette (footer.ts:1009). In the footer, isRunThemeFallback(theme) discards the fallback to keep the last-known-good theme, so the OS probe's result is thrown away. On the exact machine this PR targets — a light macOS Apple Terminal with no COLORFGBG and no OSC 11 reply — every failed runtime palette refresh now spawns /usr/bin/defaults and waits up to 400ms for nothing. Only the direct-run path consumes the appearance signal; consider skipping the probe when the fallback will be discarded.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
1 participant
Issue for this PR
Closes #736
Refs #809
Closesfor #736,Refsfor #809 — the difference is deliberate and explained in the scope note. Everything else this description mentions is named to put it out of scope, not to claim it.Type of change
What does this PR do?
The bug. Code output renders in near-white on a light terminal — readable when the session is opened in VS Code, not in the terminal itself. This is the third report of it: #617 → #704 → #736.
Why the previous two fixes did not hold. Both adjusted colour values. The defect is not a palette; it is that the mode-resolution chain in
app.tsxended in a hardcoded guess:Apple Terminal — named in #736's metadata next to "macOS Appearance: Light" — sets no
COLORFGBGand does not reliably answer the OSC 11 background query. With both signals absent the chain returns"dark", so a light-background user gets the dark palette no matter how well that palette is tuned. No colour change could close it.Two layers are fixed.
Startup detection.
resolveInitialMode()now encodes the whole chain as one pure function, ordered by how well each signal describes this terminal window: the OSC 11 reply, thenCOLORFGBG, then OS appearance, then dark as a genuine last resort. OSC outranksCOLORFGBGbecause the env var is inherited and survives ssh, tmux, sudo and profile changes;COLORFGBGnow only shortens the OSC wait to 250ms, which keeps the startup win from #704 without letting a stale value beat a live answer.detectSystemAppearance()supplies the signal that was missing — macOS leavesAppleInterfaceStyleunset in light mode, so adefaultsexit saying the key does not exist is the light answer, whileEACCES,ENOENT, a signal or a timeout mean unknown. It runs/usr/bin/defaultsso a stray binary onPATHcannot answer, and skips the probe entirely over ssh (where the appearance belongs to the remote host) and in CI.Direct-run renderer.
resolveRunThemereturned a hardcoded dark fallback on both failure exits, so a light terminal whose palette query failed got dark panels. That fallback'stextprefers the terminal's own default foreground, which on a light terminal resolves to black — black text over a hardcoded#0f172apanel is literally dark text in a dark box. The fallback is now built per mode and memoized; the dark instance is unchanged by identity, so callers comparing it withtoBeare unaffected.How did you verify your code works?
Mutation, not a green suite. Seven mutants were each confirmed to fail a test:
COLORFGBGoutranks OSC againdefaultsdefaultsfailure means lightexecFileis injected and the probe tests use a spy, so "does not spawn" is asserted rather than assumed.Suites: 288 tui, 187 cli/run, 9 direct-run theme, typecheck clean. The one failing tui test (
formats session continuation summary) fails identically onmain.Screenshots / recordings
Not a UI-layout change; the observable difference is text colour on a light terminal.
Checklist
Scope note. An audit of the whole colour-legibility area found the original grouping of these reports was wrong, so this claims only what the code supports:
fgand a missing code-block background, fixed separately in5ae5b79fcb. This change does nothing for it./usr/bin/defaults, no overriding user lock, and a theme that depends on terminal mode. The reporter's environment satisfies all four (darwin, Apple Terminal, macOS Appearance Light), and the mechanism is reproduced, so this closes it. If it recurs in an environment those conditions do not cover, that is a new report rather than this one reopening.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests